Core: Add UTs for SerializationUtil class. by pbajpai21 · Pull Request #18050 · apache/iceberg

pbajpai21 · 2026-09-10T14:03:28Z

Adds unit test coverage for SerializationUtil. Covers the byte and base64 serialize/deserialize round trips, null handling on both deserialize paths, and MIME line-wrapping in the base64 encoding.

pbajpai21 · 2026-09-11T13:45:32Z

@ebyhr @szehon-ho Please review the PR when you have time. Thank you.

ebyhr · 2026-09-14T01:10:16Z

+  @Test
+  void bytesRoundTripPreservesValue() {
+    String original = "s3://bucket/table/metadata/v1.metadata.json";
+    byte[] bytes = SerializationUtil.serializeToBytes(original);


The test coverage for objects extending HadoopConfigurable looks missing. Is it intentional?

Thank you @ebyhr for pointing this out. I have added the UTs to cover this and also covered error-handling paths.
Kindly review again. Thank you.

pbajpai21 · 2026-09-24T07:09:55Z

@ebyhr I have applied your suggestions in the code, kindly review the PR and If looks good to you, please approve.
Thank you.

laskoviymishka

Thanks for adding these — direct coverage for SerializationUtil is a genuine gap (the only production callers are in mr, and nothing in core tested it directly), so having it is a small win.

Most of the file round-trips through the JDK's own serialization and base64, which is fine but low-risk. The part I'd actually want tightened is the two HadoopConfigurable tests, since that branch is the one piece of logic unique to this class — and as written both pass without exercising it. serializeToBytesAppliesCustomConfSerializerToHadoopConfigurable returns the default SerializableConfiguration from its serializer, so it can't tell "our function's result was serialized" from "the util built its own and called the lambda incidentally"; and hadoopConfigurableRoundTripPreservesConfiguration round-trips fine even if the instanceof HadoopConfigurable branch were deleted, since the fixture wraps the conf in its constructor regardless. I left inline suggestions on both.

The rest is minor — cause-based exception assertions over hasMessage, \r\n vs \n, the (Object) casts, and the Test-prefixed fixture name. None of those block.

laskoviymishka · 2026-10-06T16:33:40Z

+    Function<Configuration, SerializableSupplier<Configuration>> confSerializer =
+        c -> {
+          confSerializerInvoked[0] = true;
+          return new SerializableConfiguration(c);


The serializer here returns a plain SerializableConfiguration, which is exactly what the fixture already uses — so this passes even if serializeToBytes dropped our function's result and built its own. The two isTrue() flags only prove the lambda ran, not that its output was the thing serialized.

I'd have the serializer return a distinctive supplier (one that yields a Configuration carrying a marker key), round-trip the bytes, and assert getConf() comes back with the marker. That's the actual serializeConfWith contract that Spark and Flink substitute their own serializers into.

Good catch, you're right that returning a plain SerializableConfiguration made it indistinguishable from the default.
Fixed: the serializer now injects a custom.serializer.marker key, and the test round-trips and asserts getConf() comes back carrying that marker. That proves our serializer's output is the thing actually serialized, not just that the lambda ran.
Thank you.

laskoviymishka · 2026-10-06T16:33:40Z

+    byte[] bytes = SerializationUtil.serializeToBytes(configurable);
+    TestHadoopConfigurable roundTripped = SerializationUtil.deserializeFromBytes(bytes);
+
+    assertThat(roundTripped.getConf().get("test.key")).isEqualTo("test.value");


The fixture constructor already wraps the conf in SerializableConfiguration, so this round-trip succeeds whether or not SerializationUtil ever enters the instanceof HadoopConfigurable branch — delete that branch and the test is still green. Since this is the only test of the default HadoopConfigurable path, I'd start the fixture from a non-serializable conf holder (a transient Configuration, or a supplier that throws NotSerializableException until serializeConfWith runs), or assert serializeConfWithInvoked after the single-arg call, so it fails when that branch regresses.

Agreed, the old fixture wrapped the conf in SerializableConfiguration in its constructor, so the branch could be deleted and the test stayed green. The fixture now holds a live, non-transient Configuration, which isn't Serializable, so the round trip only succeeds because serializeToBytes runs serializeConfWith. I verified it: with the instanceof HadoopConfigurable branch removed, this test fails with NotSerializableException: Configuration. I also kept a serializeConfWithInvoked assertion for good measure.

laskoviymishka · 2026-10-06T16:33:40Z

+    Object notSerializable = new Object();
+    assertThatThrownBy(() -> SerializationUtil.serializeToBytes(notSerializable))
+        .isInstanceOf(UncheckedIOException.class)
+        .hasMessage("Failed to serialize object");


hasMessage pins the human-readable string but never checks the cause, so a reword breaks the test while an actual regression in what gets wrapped slips through. I'd add .hasCauseInstanceOf(NotSerializableException.class) here (and StreamCorruptedException in deserializeFromBytesWrapsIOException) — strictly more informative; keep the message assertion too if you like.

Done- added hasCauseInstanceOf(NotSerializableException.class) here and hasCauseInstanceOf(StreamCorruptedException.class) in deserializeFromBytesWrapsIOException. Kept the message assertions too.

laskoviymishka · 2026-10-06T16:33:40Z

+    // the round trip verifies the MIME decoder tolerates that wrapping.
+    String original = "a".repeat(1000);
+    String encoded = SerializationUtil.serializeToBase64(original);
+    assertThat(encoded).contains("\n");


contains("\n") works since the MIME separator is CRLF, but it's vague about what we're actually pinning. I'd tighten it to the real wrapping behavior:

Suggested change

assertThat(encoded).contains("\n");

assertThat(encoded).contains("\r\n");

Applied - switched to contains("\r\n") to pin the actual MIME separator. Thanks.

laskoviymishka · 2026-10-06T16:33:40Z

+
+  @Test
+  void deserializeFromBytesReturnsNullForNullInput() {
+    assertThat((Object) SerializationUtil.deserializeFromBytes(null)).isNull();


Small thing — the (Object) cast is only here to settle generic inference. Pulling the result into a local reads cleaner (same for the base64 null test):

Suggested change

assertThat((Object) SerializationUtil.deserializeFromBytes(null)).isNull();

Object result = SerializationUtil.deserializeFromBytes(null);

assertThat(result).isNull();

Applied for both null tests- pulled the result into a local Object result instead of the cast. It reads cleaner now, Thanks.

laskoviymishka · 2026-10-06T16:33:40Z

+    assertThat(roundTripped.getConf().get("test.key")).isEqualTo("test.value");
+  }
+
+  private static class TestHadoopConfigurable implements HadoopConfigurable, Serializable {


The Test prefix on this nested helper can read as a test class to discovery tooling and reviewers. Iceberg usually names these fixtures without the leading Test — I'd call it something like HadoopConfigurableFixture.

Renamed to HadoopConfigurableFixture to avoid the Test prefix. Thanks.

developer-rpai

Meaningful round-trip assertions (not tautological), the prior reviewer's HadoopConfigurable concern was genuinely addressed, CI is fully green, no flakiness. The findings below are nits/questions; none block.

Nit: Base64.getMimeEncoder() emits CRLF line breaks, so contains("\r\n") would pin the actual MIME behavior more precisely than contains("\n").
One error path looks uncovered: deserializeFromBase64 with malformed input throws a raw IllegalArgumentException from the MIME decoder, while the byte-path failures are wrapped in UncheckedIOException. Is that asymmetry intentional? If so, a test documenting it would lock the contract in.
Minor: null is covered on both deserialize paths but not on serializeToBytes(null) (which writes and reads back null cleanly). Worth a one-liner for symmetry, or a note if intentionally omitted.
Nit: in serializeToBytesAppliesCustomConfSerializerToHadoopConfigurable, the single-threaded invocation flag works fine as a one-element array, but AtomicBoolean is the more idiomatic choice.

pbajpai21 · 2026-10-07T11:57:23Z

Thanks for adding these — direct coverage for SerializationUtil is a genuine gap (the only production callers are in mr, and nothing in core tested it directly), so having it is a small win.

Most of the file round-trips through the JDK's own serialization and base64, which is fine but low-risk. The part I'd actually want tightened is the two HadoopConfigurable tests, since that branch is the one piece of logic unique to this class — and as written both pass without exercising it. serializeToBytesAppliesCustomConfSerializerToHadoopConfigurable returns the default SerializableConfiguration from its serializer, so it can't tell "our function's result was serialized" from "the util built its own and called the lambda incidentally"; and hadoopConfigurableRoundTripPreservesConfiguration round-trips fine even if the instanceof HadoopConfigurable branch were deleted, since the fixture wraps the conf in its constructor regardless. I left inline suggestions on both.

The rest is minor — cause-based exception assertions over hasMessage, \r\n vs \n, the (Object) casts, and the Test-prefixed fixture name. None of those block.

@laskoviymishka Thank you for the thorough review. These are very valuable and insightful comments.
I have addressed all the comments. The two main ones: the Hadoop tests now genuinely fail if the instanceof HadoopConfigurable branch regresses (verified by temporarily removing it), and the custom-serializer test proves the serializer's output is what's serialized via a marker key. Details in the per-line replies.

Kindly review the latest changes again. Thank you.

pbajpai21 · 2026-10-07T12:17:56Z

Meaningful round-trip assertions (not tautological), the prior reviewer's HadoopConfigurable concern was genuinely addressed, CI is fully green, no flakiness. The findings below are nits/questions; none block.

1. Nit: `Base64.getMimeEncoder()` emits CRLF line breaks, so `contains("\r\n")` would pin the actual MIME behavior more precisely than `contains("\n")`.

2. One error path looks uncovered: `deserializeFromBase64` with malformed input throws a raw `IllegalArgumentException` from the MIME decoder, while the byte-path failures are wrapped in `UncheckedIOException`. Is that asymmetry intentional? If so, a test documenting it would lock the contract in.

3. Minor: null is covered on both deserialize paths but not on `serializeToBytes(null)` (which writes and reads back null cleanly). Worth a one-liner for symmetry, or a note if intentionally omitted.

4. Nit: in `serializeToBytesAppliesCustomConfSerializerToHadoopConfigurable`, the single-threaded invocation flag works fine as a one-element array, but `AtomicBoolean` is the more idiomatic choice.

@developer-rpai Thank you for your review and approval on PR.
I have applied \r\n and added a serializeToBytes(null) round-trip test, the boolean[] flag is already gone from the reworked HadoopConfigurable test and the base64 IllegalArgumentException is intentional - it's thrown by the JDK's Base64 decoder before bytes reach deserializeFromBytes.

laskoviymishka

both things I flagged last round landed. hadoopConfigurableRoundTripPreservesConfiguration is branch-sensitive now — the fixture holds a raw, non-serializable Configuration in a non-transient field, so deleting the instanceof HadoopConfigurable branch makes the round trip throw instead of quietly passing. That was the round-1 concern and it's genuinely fixed. The custom.serializer.marker assertion in the custom-serializer test also proves it's our serializer's output getting written, not an incidental default — my secondary ask. The rename and the (Object) cleanup are in too.

The rest is optional and none of it blocks. The .hasMessage(...) on the two exception tests is the one carryover — isInstanceOf + hasCauseInstanceOf already prove the behavior, so I'd drop the exact-wording pin. The serializeConfWithInvoked flag is redundant now that the non-serializable field and the marker prove the branch ran. And I'd sharpen the default Hadoop test's name to say it covers the SerializableConfiguration::new overload, since it and the custom-serializer test are nearly the same shape otherwise. All follow-up material.

This is in good shape now — happy to approve.

laskoviymishka · 2026-10-07T20:43:07Z

+
+  @Test
+  void deserializeFromBytesWrapsIOException() {
+    // Bytes that are not a valid object stream make ObjectInputStream throw an IOException.


This is the one carryover from last round — isInstanceOf(UncheckedIOException.class) plus hasCauseInstanceOf(...) already prove the wrapping behavior, so I'd drop the .hasMessage(...) here and on the deserialize test at line 150. Pinning the exact wrapper wording means a harmless reword breaks the test; if you'd rather keep a message check, hasMessageContaining("serialize") is looser. Non-blocking.

Done - switched to hasMessageContaining("serialize")/"deserialize"). This stops pinning the exact wording. Thanks.

laskoviymishka · 2026-10-07T20:43:07Z

+    assertThat(roundTripped.getConf().get("test.key")).isEqualTo("test.value");
+  }
+
+  private static class HadoopConfigurableFixture implements HadoopConfigurable, Serializable {


The serializeConfWithInvoked assertion doesn't add much now — the fixture's non-serializable conf field already fails serialization if the branch regresses, and the marker assertion in the custom-serializer test proves it ran. I'd drop the flag (and its field) in both Hadoop tests and let the round trip carry the check. Optional.

Done - removed the flag and field in both tests, the non-serializable conf field and the marker assertion already prove the branch ran. Thanks.

pbajpai21 · 2026-10-08T04:49:52Z

@laskoviymishka Thanks for the careful review and the approval.
Addressed all three follow-ups in the latest commit

loosened the two exception-message assertions to hasMessageContaining(...)
removed the now-redundant serializeConfWithInvoked flag and field, and
renamed the default Hadoop test to hadoopConfigurableRoundTripUsesDefaultSerializableConfiguration to flag that it covers the SerializableConfiguration::new overload.

I really appreciate the thorough feedback across both rounds.
Please review the latest changes and If you find them in good and acceptable shape, please merge the PR too. (I still don't have write access 😄 )

github-actions Bot added the core label Sep 10, 2026

pbajpai21 changed the title ~~Added UTs for SerializationUtil class.~~ Core: Add UTs for SerializationUtil class. Sep 10, 2026

ebyhr reviewed Sep 14, 2026

View reviewed changes

pbajpai21 force-pushed the serializationutil-tests branch from bcc6305 to 72dd6b8 Compare September 14, 2026 06:46

pbajpai21 requested a review from ebyhr September 14, 2026 08:25

laskoviymishka requested changes Oct 6, 2026

View reviewed changes

developer-rpai approved these changes Oct 7, 2026

View reviewed changes

github-actions Bot added API spark parquet arrow flink ORC INFRA docs build hive AWS Specification Issues that may introduce spec changes. ALIYUN GCP DELL OPENAPI AZURE KAFKACONNECT labels Oct 7, 2026

Prashant Bajpai added 3 commits October 7, 2026 10:44

Added UTs for SerializationUtil class.

4b8b1e4

Use Maps.newHashMap to satisfy checkstyle

2838393

Added UTs for HadoopConfigurable path in SerializationUtil.

f21e57e

Added UTs for error-handling paths in SerializationUtil.

77991ca

pbajpai21 force-pushed the serializationutil-tests branch from 5f44b28 to 77991ca Compare October 7, 2026 05:16

github-actions Bot removed API ORC INFRA AWS ALIYUN GCP DELL OPENAPI AZURE KAFKACONNECT labels Oct 7, 2026

Addressed PR Review comments by @laskoviymishka.

b617246

Addressed PR Review comments by @developer-rpai.

589d76b

laskoviymishka approved these changes Oct 7, 2026

View reviewed changes

Addressed PR Review comments by @laskoviymishka.

e931a6a

laskoviymishka merged commit 6f96ba3 into apache:main Oct 8, 2026
40 checks passed

	assertThat(encoded).contains("\n");
	assertThat(encoded).contains("\r\n");

	assertThat((Object) SerializationUtil.deserializeFromBytes(null)).isNull();
	Object result = SerializationUtil.deserializeFromBytes(null);
	assertThat(result).isNull();

Conversation

pbajpai21 commented Sep 10, 2026

Uh oh!

pbajpai21 commented Sep 11, 2026

Uh oh!

ebyhr Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbajpai21 Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbajpai21 commented Sep 24, 2026

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

developer-rpai left a comment

Choose a reason for hiding this comment

Uh oh!

pbajpai21 commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pbajpai21 commented Oct 7, 2026

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbajpai21 commented Oct 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ebyhr Sep 14, 2026 •

edited

Loading

pbajpai21 Sep 14, 2026 •

edited

Loading

pbajpai21 commented Oct 7, 2026 •

edited

Loading